Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge the CLI 'installValues' type with Helm 'Values' type #3291

Merged
merged 10 commits into from
Aug 21, 2019

Conversation

ihcsim
Copy link
Contributor

@ihcsim ihcsim commented Aug 20, 2019

This PR merges the cmd.installValues type with the charts.Values type. This merge ensures that all the variables and defaults defined in the CLI and Helm chart are found in one place (i.e. pkg/charts/values.go). It also moves many of the Helm-related code out of the CLI package.

Also, added unit test to validate the YAML output generated by the Helm renderer, using defaults from the values.yaml file.

Fixes #3237.

Ivan Sim added 7 commits August 19, 2019 23:09
Signed-off-by: Ivan Sim <ivan@buoyant.io>
Signed-off-by: Ivan Sim <ivan@buoyant.io>
Signed-off-by: Ivan Sim <ivan@buoyant.io>
Signed-off-by: Ivan Sim <ivan@buoyant.io>
Signed-off-by: Ivan Sim <ivan@buoyant.io>
Signed-off-by: Ivan Sim <ivan@buoyant.io>
Signed-off-by: Ivan Sim <ivan@buoyant.io>
@ihcsim ihcsim requested a review from alpeb August 20, 2019 13:57
@ihcsim ihcsim self-assigned this Aug 20, 2019
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 20, 2019

Integration test results for b871fa6: success 🎉
Log output: https://gist.github.com/1fa6a1f8011f6dc2329221e4576b1164

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome, thanks!

The new install_helm_test.go is sweet 👍 . Given the scaffolding already in place in that file, could you add a test there for HA?

Also a small TIOLI suggestion for values.go that makes it slightly simpler, avoiding a couple of pointers and dereferencing steps. Let me know what you think:

diff --git a/pkg/charts/values.go b/pkg/charts/values.go
index f8caf2a0..d9575bd2 100644
--- a/pkg/charts/values.go
+++ b/pkg/charts/values.go
@@ -223,12 +223,11 @@ func readDefaults(chartDir string, ha bool) (*Values, error) {
                        return nil, err
                }
 
-               merged, err := values.merge(v)
+               var err error
+               values, err = values.merge(v)
                if err != nil {
                        return nil, err
                }
-
-               values = *merged
        }
 
        return &values, nil
@@ -237,14 +236,13 @@ func readDefaults(chartDir string, ha bool) (*Values, error) {
 // merge merges the non-empty properties of src into v.
 // A new Values instance is returned. Neither src nor v are mutated after
 // calling merge.
-func (v Values) merge(src Values) (*Values, error) {
+func (v Values) merge(src Values) (Values, error) {
        // By default, mergo.Merge doesn't overwrite any existing non-empty values
        // in src. So in HA mode, we are merging values.yaml into values-ha.yaml
        // so that all the HA values take precedence.
        if err := mergo.Merge(&src, v); err != nil {
-               return nil, err
+               return Values{}, err
        }
 
-       newValue := src
-       return &newValue, nil
+       return src, nil
 }

cli/cmd/install_helm_test.go Show resolved Hide resolved
cli/cmd/install_helm_test.go Show resolved Hide resolved
cli/cmd/install_helm_test.go Outdated Show resolved Hide resolved
pkg/charts/values.go Outdated Show resolved Hide resolved
Ivan Sim added 3 commits August 20, 2019 12:29
Signed-off-by: Ivan Sim <ivan@buoyant.io>
Signed-off-by: Ivan Sim <ivan@buoyant.io>
Signed-off-by: Ivan Sim <ivan@buoyant.io>
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍 :shipit: 🎸

@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 20, 2019

Integration test results for c837f23: success 🎉
Log output: https://gist.github.com/ffbb082db9df2d18e380978d851394a2

@ihcsim ihcsim merged commit 183e42e into master Aug 21, 2019
@ihcsim ihcsim deleted the isim/helm-cli-values-types branch August 21, 2019 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revisit the different 'Values' types in the CLI install code
3 participants